-
Notifications
You must be signed in to change notification settings - Fork 23
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Space charge 2D envelope push #841
Space charge 2D envelope push #841
Conversation
for more information, see https://pre-commit.ci
Co-authored-by: Axel Huebl <[email protected]>
Co-authored-by: Axel Huebl <[email protected]>
Co-authored-by: Axel Huebl <[email protected]>
Co-authored-by: Axel Huebl <[email protected]>
updates: - [github.com/astral-sh/ruff-pre-commit: v0.9.4 → v0.9.6](astral-sh/ruff-pre-commit@v0.9.4...v0.9.6) Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Move "workflows" to "how-to guides" as in WarpX. Clean up some wording.
Currently, we only support (and document correctly) "static" units, but the user was able to also pass "dynamic", upon which we did print "Dynamic units" but then still used static ones. This now errors out correctly for anything else than "static" until we implement another variant.
* `Source` Element for openPMD * Rename Source Files of `BeamMonitor` * Deduplicate openPMD Logic Helpers * Docs * Example * Doc: Fix Copy-Paste
* update dashboard __init__.py import * Add tooltips functionality to non-nested states * Add tooltips for the custom vselects * update type hints
/* | ||
using SrcData = WarpXParticleContainer::ParticleTileType::ConstParticleTileDataType; | ||
tmp.copyParticles(*pc, | ||
[=] AMREX_GPU_HOST_DEVICE (const SrcData& src, int ip, const amrex::RandomEngine& engine) | ||
{ | ||
const SuperParticleType& p = src.getSuperParticle(ip); | ||
return random_filter(p, engine) * uniform_filter(p, engine) | ||
* parser_filter(p, engine) * geometry_filter(p, engine); | ||
}, true); | ||
*/ |
Check notice
Code scanning / CodeQL
Commented-out code Note
/* | ||
// comment this in once IntSoA::nattribs is > 0 | ||
|
||
std::copy(IntSoA::names_s.begin(), IntSoA::names_s.end(), int_soa_names.begin()); | ||
|
||
for (auto int_idx=0; int_idx < RealSoA::nattribs; int_idx++) { | ||
auto const component_name = int_soa_names.at(int_idx); | ||
getComponentRecord(component_name).storeChunkRaw( | ||
soa.GetIntData(int_idx).data(), {offset}, {numParticleOnTile64}); | ||
} | ||
*/ |
Check notice
Code scanning / CodeQL
Commented-out code Note
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks awesome!
Added small inline comments.
if (space_charge_model == "3D") { | ||
pp_dist.query("charge", intensity); | ||
throw std::runtime_error("3D space charge model not yet implemented in envelope mode."); | ||
} else if (space_charge_model == "2D") { | ||
pp_dist.query("current", intensity); | ||
} else { | ||
throw std::runtime_error("Unknown space_charge_model (use '2D' or '3D') "); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I want to unify this with algo.space_charge
. There is no need for two parameters, we can just do algo.space_charge = false
(default), = 2D
and = 3D
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes sense, although it will break the existing space charge examples, where algo.space_charge = true
. In that case, maybe we should push the change through as a follow-up, since it also impacts the logic outside the envelope tracking?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I can implement it in a backwards compatible way, will give it a try tomorrow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I pushed a backwards compatible change and it currently passes CI. This is what existing users will see (True defaults now to 3D space charge and raises a warning to use the new syntax.)
I will push another commit now that updates all examples to the new syntax, so people see and use that going forward. After a few months (year), we can drop support for the old syntax. I added an inline TODO where we will remove it.
04dd198
to
e45cafd
Compare
examples/aperture/input_aperture.in
Outdated
@@ -40,7 +40,7 @@ collimator.aperture_y = 1.5e-3 | |||
# Algorithms | |||
############################################################################### | |||
algo.particle_shape = 2 | |||
algo.space_charge = false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have one concern about this recent change. With algo.space_charge = false
, it is obvious to the user that space charge is not active. Without this line, I think it is too easy to forget that space charge is off by default, especially when a nonzero bunch charge is provided. This is especially important in the examples. If we keep three string input options "2D", "3D", and "false", then we could keep this line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was wondering about that, good point.
Off was and is the default; I agree we can keep it explicit to avoid any confusion.
src/python/ImpactX.cpp
Outdated
pp_algo.add("space_charge", std::string("3D")); | ||
} else { | ||
py::print("sim.space_charge = False is deprecated, please use space_charge = \"off\" (or skip for default)"); | ||
pp_algo.add("space_charge", std::string("off")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I see that the "off" string is supported, so I would keep this in the example input files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. And we can still use false
as well.
The problem why I need to internally call this off
in the enum is that false
is a C++ keyword.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, can name it False
internally (uppercase)
3acf310
to
716c149
Compare
716c149
to
6e65816
Compare
And add doxygen and improve case matching.
This PR implements the linear push of the covariance matrix in the presence of 2D space charge.
This addresses Issue #826 in the 2D (envelope) case. Follow-up: a corresponding model for 3D space charge.